Conversation
| /// as an argument. | ||
| /// | ||
| /// The [`into_pyobject`][IntoPyObject::into_pyobject] method is designed for maximum flexibility and efficiency; it | ||
| /// - allows for a concrete Python type to be returned (the [`Target`][IntoPyObject::Target] associated type) |
There was a problem hiding this comment.
Need to finish this list, had to run from my pc just now
| /// This is typically only useful when the resulting output is going to be passed | ||
| /// to another function that only needs to borrow the output. | ||
| #[inline] | ||
| fn into_bound_object_py_any( |
There was a problem hiding this comment.
The name of this method is open to bikeshedding, I think I'm happy with this but there's probably a better one.
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
I guess we still want to tell users about BoundObject in the IntoPyObject docs in the meantime, but it looks like trait probably covers most uses I've seen in downstream code, so the docs I'm adding in #4703 will probably need an update in this PR
| Ok(obj) => Ok(obj.into_any()), | ||
| Err(err) => Err(err.into()), | ||
| } | ||
| } |
There was a problem hiding this comment.
I think into_unbound_py_any would also probably be useful (there's a customer in Coroutine::new it looks like).
There was a problem hiding this comment.
That seems reasonable to me; it certainly is common to .unbind() in pydantic-core (so that data can be stored in structs, mostly).
|
I realise that this is non-breaking to add in 0.23.1 as long as we delay prelude addition to 0.24. So while I think something like this is helpful both internally and for users, there is no need to rush a merge. (Especially as users can just copy this trait into their crate downstream in the meanwhile.) |
|
@Icxolu would love to hear your thoughts on this idea. Do you see alternative ways we could offer simple conveniences on top of |
|
I was thinking about this for a while now. I agree we should offer some convieniences here, this patter does come up alot. I'm still struggeling a bit with the names. The current ones feel either a bit imprecise or a bit long to me (but I don't have suggestions that I'm completely happy with either). From a functionality point of view I would have 3 methods ins mind:
Other combinations feel less common and should probably go through |
|
Thanks, I am happy to go with those names with one small adjustment; I went for Agreed about keeping it a separate trait, I had the same thinking. I named your option 3 I think there's some more additional documentation and internal uses to add, if you're happy with this current set of names I'll seek to finish up and push later. |
|
I also think there is probably space for a separate |
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
LGTM. I think the diff shows nicely already that this does decrease boilerplate and improves readability. I'm reasonable happy with the names and if other suggestions come up at some point, we can always refactor then.
I also think there is probably space for a separate IntoPyObjectInfallible trait, but I would prefer to wait with that to see how the 2024 edition changes interactions with ! and Infallible.
I agree. I think min_exhaustive_patterns (1.82+) already helps alot with this and we can afford to wait and see what the 2024 editions brings to the table.
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
This looks much nicer overall. Just a small doc suggestion.
Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
* add `IntoPyObjectExt` trait * adjust method names, more docs & usage internally * more uses of `IntoPyObjectExt` * guide docs * newsfragment * fixup doctest * Update guide/src/conversions/traits.md Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> --------- Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>
* add `IntoPyObjectExt` trait * adjust method names, more docs & usage internally * more uses of `IntoPyObjectExt` * guide docs * newsfragment * fixup doctest * Update guide/src/conversions/traits.md Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com> --------- Co-authored-by: Icxolu <10486322+Icxolu@users.noreply.github.com>
3 participants
Possibly related to #4703
I was wondering how we might be able to simplify uses of
IntoPyObjectin generic code. This PR proposes a helperIntoPyObjectExttrait which adds some methods which erase the concrete python type and force creation ofPyErrresults.Overall this seems like a good amount of simplification at call sites, so I'll probably use this in
pydantic-coreif we don't land this here until 0.24.Draft because the docs are unfinished and there's also a ton more locations internally this can be used. The kids are sick and need me to comfort them a lot tonight so I think I'm done for the day.